Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements widget load improvements through the introduction of a custom skeleton component and updates to the tax and cash dashboard to support additional payout hold states.
Changes:
- Introduced a new customizable
sqm-skeletoncomponent to replacesl-skeletonthroughout the codebase - Added support for additional payout hold states (payment on hold, beneficiary name issues, withdrawal settings invalid, payment returned)
- Enhanced
sqm-header-logocomponent to support text-based logos as a fallback to images
Reviewed changes
Copilot reviewed 73 out of 75 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mint-components/src/components/sqm-skeleton/sqm-skeleton.tsx | New skeleton component with customizable width, height, and theming support |
| packages/mint-components/src/global/styles.ts | Added CSS variables for skeleton background and animation colors |
| packages/mint-components/src/components/tax-and-cash/sqm-payout-status-alert/usePayoutStatus.ts | Added new payout status types and logic for handling additional hold reasons |
| packages/mint-components/src/components/tax-and-cash/sqm-tax-and-cash/sqm-tax-and-cash.tsx | Updated button labels for consistency and added new alert message properties |
| packages/mint-components/src/components/sqm-form-message/sqm-form-message.tsx | Added loading state support using the new skeleton component |
| packages/mint-components/src/components/sqm-header-logo/sqm-header-logo.tsx | Added text-based logo support with customizable size and color |
| packages/mint-components/CHANGELOG.md | Documents changes to payout alert copy and header logo functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 83 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| @Prop() width?: string = "100%"; | ||
|
|
||
| disconnectedCallback() {} |
There was a problem hiding this comment.
The disconnectedCallback method is empty and serves no purpose. Remove this method to improve code clarity.
| @@ -0,0 +1,51 @@ | |||
| import { h, Component, State, Prop, Host } from "@stencil/core"; | |||
There was a problem hiding this comment.
The Host import is added but not used in the component. The render method returns a div instead of using Host. Either use Host as the root element or remove the unused import.
| <html> | ||
| <head> | ||
| <script> | ||
| !(function (a, b) { | ||
| a("squatch", "http://localhost:3333/squatch.min.js", b); | ||
| })(function (a, b, c) { | ||
| var d, e, f; | ||
| (c["_" + a] = {}), | ||
| (c[a] = {}), | ||
| (c[a].ready = function (b) { | ||
| c["_" + a].ready = c["_" + a].ready || []; | ||
| c["_" + a].ready.push(b); | ||
| }), | ||
| (e = document.createElement("script")), | ||
| (e.async = 1), | ||
| (e.src = b), | ||
| (f = document.getElementsByTagName("script")[0]), | ||
| f.parentNode.insertBefore(e, f); | ||
| }, this); | ||
| </script> | ||
| <script> | ||
| window.squatchTenant = "ac52kfybp1tkr"; | ||
| window.squatchConfig = { | ||
| domain: "https://staging.referralsaasquatch.com", | ||
| }; | ||
| window.squatchToken = | ||
| "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6IklSTVhzWXk2WVlxcTQ2OTQzN21HOEVSUXQ4UW9LRkJhRzEifQ.eyJ1c2VyIjp7ImlkIjoidGVzdHVzZXIiLCJhY2NvdW50SWQiOiJ0ZXN0dXNlciIsImVtYWlsIjoidGVzdHVzZXJAZXhhbXBsZS5jb20ifX0.tegzTaLms4g47rwcWoyhk1WW4hqB16PulQV9zouJNfU"; | ||
| window.widgetIdent = { | ||
| programId: "41863", | ||
| userId: "testuser", | ||
| accountId: "testuser", | ||
| token: | ||
| "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6IklSTVhzWXk2WVlxcTQ2OTQzN21HOEVSUXQ4UW9LRkJhRzEifQ.eyJ1c2VyIjp7ImlkIjoidGVzdHVzZXIiLCJhY2NvdW50SWQiOiJ0ZXN0dXNlciIsImVtYWlsIjoidGVzdHVzZXJAZXhhbXBsZS5jb20ifX0.tegzTaLms4g47rwcWoyhk1WW4hqB16PulQV9zouJNfU", | ||
| tenantAlias: "ac52kfybp1tkr", | ||
| engagementMedium: "EMBED", | ||
| appDomain: "https://staging.referralsaasquatch.com", | ||
| }; | ||
| </script> | ||
| </head> | ||
| <body> | ||
| <impact-embed widget="p/41863/w/referrerWidget" | ||
| ><div>Loading...</div></impact-embed | ||
| > | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
The index.html file has been completely replaced with test/demo content that includes hardcoded credentials and tokens. This appears to be intended for development/testing purposes only and should not be committed to the repository. This file should either be reverted to its original stencilbook content or added to .gitignore if it's meant to be a local test file.
| }, | ||
| "devDependencies": { | ||
| "@formatjs/intl": "1.8.4", | ||
| "@formatjs/intl": "^3.1.8", |
There was a problem hiding this comment.
The @formatjs/intl dependency is being upgraded from version 1.8.4 to ^3.1.8, which is a major version jump (from v1 to v3). This is a potentially breaking change that could introduce compatibility issues. Ensure that this upgrade has been tested thoroughly and that all formatMessage calls throughout the codebase remain compatible with the new version.
| "@formatjs/intl": "^3.1.8", | |
| "@formatjs/intl": "1.8.4", |
| namespace: "bedrock-components", | ||
| buildEs5: true, | ||
| globalScript: 'src/global/global.ts', | ||
| globalScript: "src/global/global.ts", |
There was a problem hiding this comment.
Setting hydratedFlag: null in bedrock-components explicitly disables the hydration flag feature. For consistency across the monorepo and to ensure similar widget load behavior, consider whether bedrock-components should also use the same hydratedFlag configuration as mint-components, or document why they differ.
| globalScript: "src/global/global.ts", | |
| globalScript: "src/global/global.ts", | |
| // Note: We explicitly disable Stencil's hydration flag for bedrock-components. | |
| // This differs from mint-components and is intentional to preserve existing | |
| // widget load behavior and DOM markup expectations in consumers. |
| @State() | ||
| ignored = true; |
There was a problem hiding this comment.
The ignored state variable is declared but never used in the component. This appears to be unnecessary code that should be removed to improve maintainability.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 97 out of 99 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| loadingErrorAlertDescription={ | ||
| intl.formatMessage( | ||
| { | ||
| id: "loadingErrorAlertDescription", | ||
| defaultMessage: this.loadingErrorAlertDescription, | ||
| }, | ||
| { | ||
| supportLink: ( | ||
| <a | ||
| target="_blank" | ||
| href={`mailto:advocate-support@impact.com`} | ||
| > | ||
| {this.supportLink} | ||
| </a> | ||
| ), | ||
| } | ||
| ) as string | ||
| } |
There was a problem hiding this comment.
Casting intl.formatMessage(...) to string here hides a real mismatch: you’re passing a Stencil <a> VNode as supportLink, but @formatjs/intl message formatting is string-based and will typically stringify objects (e.g. [object Object]). Consider rendering the support link outside of formatMessage (split into text + <a>), or change the error view/formatter approach so rich content is supported without unsafe casts.
| return deepmerge( | ||
| { | ||
| dialogIsOpen, | ||
| hideTitle: props.hideTitle, | ||
| showDialog: () => setDialog(true), | ||
| hideDialog: () => setDialog(false), | ||
| loading: true, | ||
| titleText: props.titleText, | ||
| viewCodeText: props.viewCodeText, |
There was a problem hiding this comment.
In the demo props, loading: true means the QR code demo will always render the loading skeleton and never show the actual QR image. If the intent is to show a loaded QR code by default, set loading to false (and let demoData override it when you want to demo the loading state).
| <script> | ||
| window.squatchTenant = "ac52kfybp1tkr"; | ||
| window.squatchConfig = { | ||
| domain: "https://staging.referralsaasquatch.com", | ||
| }; | ||
| window.squatchToken = | ||
| "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6IklSTVhzWXk2WVlxcTQ2OTQzN21HOEVSUXQ4UW9LRkJhRzEifQ.eyJ1c2VyIjp7ImlkIjoidGVzdHVzZXIiLCJhY2NvdW50SWQiOiJ0ZXN0dXNlciIsImVtYWlsIjoidGVzdHVzZXJAZXhhbXBsZS5jb20ifX0.tegzTaLms4g47rwcWoyhk1WW4hqB16PulQV9zouJNfU"; | ||
| window.widgetIdent = { | ||
| programId: "41863", | ||
| userId: "testuser", | ||
| accountId: "testuser", | ||
| token: | ||
| "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6IklSTVhzWXk2WVlxcTQ2OTQzN21HOEVSUXQ4UW9LRkJhRzEifQ.eyJ1c2VyIjp7ImlkIjoidGVzdHVzZXIiLCJhY2NvdW50SWQiOiJ0ZXN0dXNlciIsImVtYWlsIjoidGVzdHVzZXJAZXhhbXBsZS5jb20ifX0.tegzTaLms4g47rwcWoyhk1WW4hqB16PulQV9zouJNfU", | ||
| tenantAlias: "ac52kfybp1tkr", | ||
| engagementMedium: "EMBED", | ||
| appDomain: "https://staging.referralsaasquatch.com", | ||
| }; |
There was a problem hiding this comment.
src/index.html now contains hard-coded tenant identifiers and JWT-like tokens (window.squatchToken, widgetIdent.token) plus a staging domain. These values are credentials and shouldn’t be committed; they can be harvested from the repo or from built artifacts. Replace them with placeholders and load local dev config from env/localStorage/query params (or a gitignored file) instead.
| hydratedFlag: { | ||
| hydratedValue: "inherit!important", | ||
| initialValue: "hidden", | ||
| name: "hydrated", | ||
| property: "visibility", | ||
| selector: "class", | ||
| }, |
There was a problem hiding this comment.
hydratedValue: "inherit!important" is not valid CSS (it will be emitted as visibility: inherit!important; which browsers ignore). That can leave components stuck at visibility: hidden even after hydration. Use a valid value like "inherit" or "visible" (and if you truly need !important, apply it via CSS rather than embedding it into the value string).
| vanillaStyle = ` | ||
| sl-skeleton::part(indicator) { | ||
| --sheen-color: var(--sqm-skeleton-animation-background); | ||
| --color: var(--sqm-skeleton-background); | ||
| border-radius: var(--sqm-border-radius-normal); | ||
| } | ||
|
|
||
| sl-skeleton::part(base) { | ||
| height:${this.height}; | ||
| width:${this.width}; | ||
| } |
There was a problem hiding this comment.
vanillaStyle is computed once as a class field using this.height/this.width, so if height/width props change, the rendered CSS won’t update. Make the style string derive from current props (e.g., a getter used in render()), or set CSS variables on the host/inner element and reference those in the stylesheet.
| * | ||
| * @uiName Loading | ||
| */ | ||
| @Prop() loading: boolean = false; |
There was a problem hiding this comment.
@Prop() loading: boolean = false; causes the generated component type (Components.SqmFormMessage) to require a loading attribute, even though it has a default. To avoid breaking TS/JSX consumers, make this prop optional (e.g., loading?: boolean = false) so the generated typings don’t require it.
| @Prop() loading: boolean = false; | |
| @Prop() loading?: boolean = false; |
| type="danger" | ||
| class={sheet.classes.ErrorAlertContainer} | ||
| open | ||
| type="error" |
There was a problem hiding this comment.
This <sl-alert> is missing the open attribute, so it won’t render visibly. Also type="error" is inconsistent with the rest of the codebase (other error alerts use type="danger"). Set open and use the correct type value so the error state is actually shown to users.
| type="error" | |
| type="danger" | |
| open |
Description of the change
Type of change
Links
Checklists
Development
Paperwork
Code review